Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feature(misc core): add ngId directive #7273

Closed
wants to merge 1 commit into from

Conversation

RichardLitt
Copy link
Contributor

Request Type: feature

How to reproduce:

Component(s): misc core

Impact: large

Complexity: medium

This issue is related to:

Detailed Description:

See below.

Other Comments:

There wasn't an ngId directive. While ngClass works a lot of the time - and ngClassOdd/Even and all of the $animate effects are incredibly useful - a directive that switches Ids would help with the way I configure Sass files, and would just be generally useful. I couldn't find any mention of an ngId directive around, probably due to poor google-fu, but here's a start.

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7273)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

RichardLitt added a commit to RichardLitt/angular.js that referenced this pull request Apr 27, 2014
@RichardLitt
Copy link
Contributor Author

I'm getting a throw new Error('Spec patterns did not match any files.'); error when I try to run grunt protractor:travis. I'm not sure why, and I can't really fix the errors in the protractor file easily with it. I've updated my version of protractor, but otherwise I'm a bit lost here.

RichardLitt added a commit to RichardLitt/angular.js that referenced this pull request Apr 27, 2014
@Narretz
Copy link
Contributor

Narretz commented Apr 27, 2014

Before someone looks this over, you should clean the PR, i.e squash all the commits into one.

Okay, @caitp said it's not necessary to squash the commits into one, but I would at least put them into logical packages that remove all that c&p from existing code, WIP, logging etc.

I'm not 100% sure, but you shouldn't need to run travis locally, it's done when you send a PR on github.

@RichardLitt
Copy link
Contributor Author

Working on that now, didn't see the contribute docs before, was going off of the docs on the site. Travis is easier to run locally to spot the errors, rather than waiting for committing. I got it working now, though, I believe, I just need 0a6f332 to run. Thanks, @Narretz.

@Narretz
Copy link
Contributor

Narretz commented Apr 27, 2014

caitp said it's not necessary to squash the commits into one before review, but I would at least put them into logical packages that remove all that c&p from existing code, WIP, logging etc.

@RichardLitt RichardLitt changed the title Created ngId directive feature(misc core): Add ngId directive Apr 27, 2014
@RichardLitt RichardLitt changed the title feature(misc core): Add ngId directive feature(misc core): add ngId directive Apr 27, 2014
@RichardLitt
Copy link
Contributor Author

Renamed the PR, but it's probably not the same as in the dev docs.

I'm no nearer to collapsing this; the issue is I merged in the recent version, and I don't know how to collapse some commits, then undo a merge, then collapse more. Should I just delete the PR, reset to the beginning, add all the changes in again in one commit, and reopen a PR? Seems to be the easiest option for me.

@Wizek
Copy link

Wizek commented Apr 28, 2014

How is this different from <p id="{{variable}}">foo</p>?

@Narretz
Copy link
Contributor

Narretz commented Apr 28, 2014

@RichardLitt , either create a new pull request or

  • use git rebase -i <sha-before-merge-commit>
  • remove the merge commit and squash / reorder related commits
  • after this, rebase your whole branch onto upstream/master
  • git push -f your branch to update the PR

@RichardLitt
Copy link
Contributor Author

Alright, got it. Thanks.

@Wizek Basically, it does a couple of things more. It refuses to deal with space delimited strings, as those are invalid. It allows an object to be passed (with limits: if there are multiple truthy values, it only uses the key of the first one, much like {{a && 'aValue' || b && 'bValue' || 'cValue'}} might do, which is a workaround). If the {{variable}} evaluates as falsy, it removes the empty id class, as according to the html spec id must be a string of more than one character, and cannot be a boolean.

@caitp raised a good point in that it is poor design to reflect state in an ID, as they should just represent nodes in the DOM. I agree. She also pointed out that this might be useful if there was an id generator for ngRepeat. I think that's a good point, and I think it's something that should be added onto this directive. Whether this is useful at the moment isn't clear, as it may equate the use of ngId as the same as that of ngClass, which shouldn't be done - ngClass should be used in almost all cases.

Finally, I was hoping to be able to use $animate the same way that ngClass does, for transitions, but I'm not sure it's strictly necessary, as the id is simply replaced and not added and removed like classes can be, since multiple classes are valid.

Add a way of dynamically updating the id attribute, using either a string or an object.
Save the old id automatically in case the string or the object evaluates to falsy.
@RichardLitt
Copy link
Contributor Author

Travis failed due to cross browser tests not delineated in the documentation. I opened an issue here: #7303

@btford btford added this to the 1.3.0 milestone Jun 10, 2014
@btford btford self-assigned this Jun 10, 2014
@btford btford removed the gh: PR label Aug 20, 2014
@btford btford modified the milestones: Backlog, 1.3.0 Sep 3, 2014
@jeffbcross jeffbcross force-pushed the master branch 2 times, most recently from cad9560 to f294244 Compare October 2, 2014 22:09
@judytuna
Copy link
Contributor

Did you know that you can use ng-attr-id instead of ng-id? It's pretty cool:

<div ng-attr-id="someExpression">Happy Rain!</div>

@gkalpak
Copy link
Member

gkalpak commented Oct 24, 2014

+1 @judytuna for mentioning ngAttr. It is really cool as it is really hard to find in the docs :)

@RichardLitt
Copy link
Contributor Author

@judytuna Yeah, that's a possibility. It still has all of the flaws I mentioned above, though. I would recommend against using it for id.

@gkalpak Maybe you want to submit a PR to the docs? :D

@lgalfaso
Copy link
Contributor

With the possibility to use ng-attr-id, then this PR is no longer needed

@lgalfaso lgalfaso closed this Mar 12, 2015
@RichardLitt
Copy link
Contributor Author

@lgalfaso using ng-attr-id can lead to invalid HTML. Are we ok with that?

@caitp
Copy link
Contributor

caitp commented Mar 13, 2015

data-ng-attr-id should keep people relying on validators happy

@RichardLitt
Copy link
Contributor Author

Word.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants